Skip to content

Conversation

ThomasVitale
Copy link
Contributor

Fixes gh-553

}

public Builder withOptions(Map<String, Object> options) {
Objects.requireNonNullElse(options, "The options can not be null.");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed the method name here to prevent NullPointerExceptions

@tzolov
Copy link
Contributor

tzolov commented Apr 6, 2024

Hey @ThomasVitale , thanks for improving this.

It looks related to the aca8ffd PR I've just merged, but concerns the legacy generate endpoint only?

Though the ChatClient impl. uses only the new chat completion endpoint I guess we should to keep the entire OllamaApi up to date.
Could you please rebase and resolve the possible conflicts?

@tzolov
Copy link
Contributor

tzolov commented Apr 6, 2024

yeah, I did a mistake with my previous merge. Please rebase after 15628a3

@ThomasVitale
Copy link
Contributor Author

@tzolov thanks, I'll rebase the PR shortly. I can see that a template field has been added as an option to the ChatRequest model, but it doesn't look like it's part of the Ollama API for the chat completion endpoint. It's only used in the generate endpoint. (https://github.com/ollama/ollama/blob/main/api/types.go#L40)

The Ollama documentation includes it also for the chat completion endpoint, but that might be a copy/paste mistake in the docs. Looking at the code of the Ollama server, the handler for chat completion requests is always using the template configured in the model and doesn't allow overwriting (https://github.com/ollama/ollama/blob/main/server/routes.go#L1292). From what I could test, the template field included in the ChatRequest Java class will simply be ignored by Ollama. I raised the issue on the Ollama project: ollama/ollama#3514

@ThomasVitale
Copy link
Contributor Author

I've just rebased the PR on top of the current main.

@tzolov
Copy link
Contributor

tzolov commented Apr 6, 2024

Sorry for the mess I caused earlier today. Obviously lacking testing for the options/parameter merging/mapping logic. Will add some today.
Thanks again to for the alignment.

@tzolov tzolov merged commit 4e473aa into spring-projects:main Apr 6, 2024
@tzolov tzolov added this to the 1.0.0-M1 milestone Apr 6, 2024
@ThomasVitale
Copy link
Contributor Author

@tzolov no problem! Thanks for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Ollama] Missing fields in API

4 participants